Update .claude with 11 changed files (#4483) by rysweet · Pull Request #4495 · rysweet/Simard

rysweet · 2026-07-23T06:16:58Z

Summary

Concise workflow-generated PR for .claude.

Issue

Changed files

.claude/.claude-4483-stash/runtime/launcher_context.json
.claude/.claude-4483-stash/runtime/metrics/post_tool_use_metrics.jsonl
.claude/.claude-4483-stash/runtime/sessions.jsonl
docs/concepts/typed-outcome-ledger-shared-connection.md
docs/howto/diagnose-typed-outcome-database-is-locked.md
docs/index.md
docs/reference/typed-outcome-ledger-connection-registry-api.md
mkdocs.yml
src/ooda_actions/advance_goal/typed_goal_session.rs
src/typed_ooda/ledger.rs
tests/typed_ooda_outcome_lock_regression.rs

Diff stat

 .../runtime/launcher_context.json                  |   9 +
 .../runtime/metrics/post_tool_use_metrics.jsonl    |  22 ++
 .claude/.claude-4483-stash/runtime/sessions.jsonl  |   2 +
 .../typed-outcome-ledger-shared-connection.md      | 144 +++++++++
 .../diagnose-typed-outcome-database-is-locked.md   | 156 ++++++++++
 docs/index.md                                      |   1 +
 ...typed-outcome-ledger-connection-registry-api.md | 329 +++++++++++++++++++++
 mkdocs.yml                                         |   3 +
 .../advance_goal/typed_goal_session.rs             |   7 +-
 src/typed_ooda/ledger.rs                           | 197 ++++++++----
 tests/typed_ooda_outcome_lock_regression.rs        | 216 ++++++++++++++
 11 files changed, 1031 insertions(+), 55 deletions(-)

Behavior

Implemented behavior through these branch commits:
cde7b9c docs: correct busy_backoff backoff-sequence comment
3f69bbd wip: checkpoint after implementation (steps 7-8)
9d59c43 fix(typed-ooda): serialize outcome ledger through one shared connection (#4483)
1463093 docs(typed-ooda): spec-first docs for shared-connection ledger fix (#4483)

Validation

ℹ NODE_OPTIONS=--max-old-space-size=32768 (saved preference). To change: /home/azureuser/.amplihack/config
I'll start by checking the branch and the changes made so far.

● Get branch and changed files (shell)
│ cd /home/azureuser/src/Simard-deploy-4049/worktrees/feat/issue-4483-fix-systemic-typed-outcome-p
│ ersistence-failed-data && git branch --show-current && echo "---CHANGED FILES---" && git
│ --no-pager diff --stat HEAD~5..HEAD 2>/dev/null | tail -30 && echo "---STATUS---" && git
│ --no-pager status --short
└ 26 lines…

Changes are committed. Let me examine the actual fix and the new regression test.

Risk

No high-risk subsystem pattern detected from changed paths.

Checklist

Branch has 4 commit(s) ahead of main
Code review completed
Philosophy check passed

This PR was created as a draft for review before merging.

Step 16b: Outside-In Testing Results

Tested from the PR branch (feat/issue-4483-fix-systemic-typed-outcome-persistence-failed-data) as a consumer of the typed-outcome ledger would exercise it.

Detected toolchain: Rust CLI (Cargo.toml at repo root, Cargo.lock present, cargo 1.95.0 / rustc 1.95.0). Per the qa-team skill's repo-type detection, Rust CLI repos validate via native cargo test rather than the gadugi-agentic-test framework.

Chosen strategy: Native cargo test at the library/integration boundary — the same public API (CapabilityHandler, OutboxWorker) that OODA cycles and outbox startup-recovery use — plus cargo clippy as the CI lint gate. This exercises the fix through the real external consumer boundary (concurrent handlers on one DB file), which is exactly where issue #4483 manifested.

#	Scenario	Command	Result	Key output
1 (simple/regression)	Concurrent OODA cycles (112 goals) + 16 startup-recovery workers race on one ledger file; assert no `database is locked` and every terminal is durably persisted	`cargo test --test typed_ooda_outcome_lock_regression --locked`	✅ PASS	`test concurrent_cycles_and_startup_recovery_never_lock_the_outcome_ledger ... ok` — `1 passed; 0 failed` in 3.11s
2 (edge/integration)	Full typed-OODA ledger + executor + outbox behavior suite (session scoping, engineer claims/leases, approvals, terminal readback, effect drain/retry) to confirm shared-connection serialization preserves existing behavior	`cargo test --lib typed_ooda --locked`	✅ PASS	`50 passed; 0 failed; 0 ignored` in 0.98s
3 (lint gate)	Clippy on library + all tests (CI gate)	`cargo clippy --lib --tests --locked`	✅ PASS	`Finished dev profile` — no warnings

Fix count during outside-in testing: 0 (all scenarios passed on the first run; no diagnose→fix→retry iterations were required).

PR CI status at time of testing: MERGEABLE; security/audit/deny/vet/npm-audit/scripts-tests checks passing, with pre-commit and coverage still running.

Verification notes: No stray print!/println!/eprintln!/dbg! in the changed files — the former eprintln! at typed_goal_session.rs:151 now routes through tracing::error!. The regression asserts both the absence of lock errors and durable persistence of every terminal, so the fix cannot pass by silently dropping writes.

…4483) Recreate and refine the three Diátaxis docs for the typed-outcome ledger shared-connection fix, applying architect review feedback. - reference/typed-outcome-ledger-connection-registry-api.md — path-keyed OnceLock<Mutex<HashMap<PathBuf, Arc<Mutex<Connection>>>>> registry, apply_pragmas (WAL + busy_timeout=5000 + foreign_keys=ON), with_busy_retry/is_sqlite_busy bounded backoff, unchanged open() surface, lock ordering, error semantics, security notes, regression contract. - concepts/typed-outcome-ledger-shared-connection.md — the why: concurrent independent connections to one ledger file caused the SQLITE_BUSY burst; one connection per file removes it by construction. Goal-ids anonymized. - howto/diagnose-typed-outcome-database-is-locked.md — operator runbook. Architect refinements applied: - status: "design — not yet implemented" (code is pre-fix); explicit spec-first callout that docs + implementation land in the same PR. - Anonymized production goal-ids in the concept doc. - Snippet-drift disclaimer in the reference doc. - Kept retry-constant hedging; noted busy_timeout rusqlite/PRAGMA equivalence. Wired into docs/index.md and mkdocs.yml (Concepts/Reference/How-to nav). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…on (#4483) Concurrent OODA cycles across goals plus the outbox startup-recovery path each opened an independent SQLite connection to the same typed-outcome DB. Under WAL, only one writer holds the file lock at a time; the racing first-init + write acquisitions exceeded the 5s per-connection busy_timeout and surfaced "typed outcome persistence failed: database is locked". Route every handle to a typed-outcome DB path through a process-wide, path-keyed shared-connection registry (Arc<Mutex<Connection>>), so all cycles and startup recovery serialize on a single WAL + busy_timeout connection instead of contending on the file-level write lock. Pragmas (foreign_keys, busy_timeout=5s, journal_mode=WAL) are applied unconditionally at connect. Immediate write transactions gain a bounded, fail-visible backoff retry on SQLITE_BUSY/"database is locked" as belt-and-suspenders for cross-process contention; on exhaustion the persistence error is surfaced unchanged (no silent fallback). Startup recovery reuses the same handler, so it now shares the serialized connection and completes. The eprintln! recovery-incomplete log is routed through structured tracing::error! with goal_id + error fields. Public API is unchanged. Adds a concurrent-cycles + startup-recovery regression test asserting zero lock errors and durable persistence. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Automatic checkpoint to preserve work in progress. Tests and implementation saved before refactoring phase.

The doc comment claimed a 100ms cap was reached between busy-retry attempts. With BUSY_RETRY_MAX_ATTEMPTS=5 the macro returns before sleeping on attempt 5, so busy_backoff is only invoked for attempts 1..=4, yielding 10/20/40/80ms. Clarify that the 100ms cap is an overflow guard and unreachable at the current max attempts. Addresses the sole recurring (non-blocking) review note on #4483. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

rysweet

Step 17b — Comprehensive Code Review

Verdict: ✅ APPROVE / merge-ready (non-blocking observations only)

Reviewed the full diff (src/typed_ooda/ledger.rs, src/ooda_actions/advance_goal/typed_goal_session.rs, new regression test, docs) against issue #4483 and the resolved design decisions D1–D8.

Verification performed (fresh, on `cde7b9c9`)

Check	Command	Result
Regression test	`cargo test --test typed_ooda_outcome_lock_regression --locked`	✅ `1 passed` (2.88s)
Typed-OODA suite	`cargo test --lib typed_ooda --locked`	✅ `50 passed; 0 failed`
Lint gate	`cargo clippy --lib --tests --locked`	✅ no warnings
Registry routing	`grep Connection::open src/typed_ooda/`	✅ exactly one — inside `shared_connection` factory (line 88); no bypassing opens remain
No stray IO	grep `print!/println!/eprintln!/dbg!` in changed files	✅ none; `typed_goal_session.rs:151` now routes through `tracing::error!` with structured `goal_id`+`error`

Strengths

Correct root-cause fix. The path-keyed OnceLock<Mutex<HashMap<PathBuf, Arc<Mutex<Connection>>>>> registry (D1/D2) serializes every write per DB file through one WAL + busy_timeout connection, eliminating the file-level write-lock race that produced database is locked on restart bursts. Path-keying correctly preserves per-test isolation.
Fail-visible, never-swallowed errors. begin_immediate! retries only on genuine SQLITE_BUSY/DatabaseLocked (is_sqlite_busy), bounded at 5 attempts, and surfaces the unchanged persistence() error on exhaustion — outward behavior identical to the prior .map_err(persistence)?.
Pragmas applied unconditionally at connect (D3): foreign_keys=ON, busy_timeout=5s, journal_mode=WAL.
Strong regression test. Asserts BOTH the absence of lock errors AND durable read-back of every terminal — so the fix cannot pass by silently dropping writes. 112 concurrent goals + 16 startup-recovery workers reproduce the post-restart burst.
Public API unchanged (D7): CapabilityHandler::open/with_engineer_liveness signatures identical; additive & non-breaking.
Docs (concept/howto/reference) added and wired into mkdocs.yml.

Non-blocking observations

canonical_key relative-path aliasing. For a bare filename ("outcomes.sqlite3") the parent is empty → filtered out → key falls back to the raw relative path. But "./outcomes.sqlite3" has parent ".", which canonicalizes to an absolute dir → a different key for the same file, yielding two connections and defeating serialization. Production opens the ledger via a stable absolute path so this is latent, but consider canonicalizing the empty-parent case against CWD to close the gap defensively.
Process-wide poison blast radius. With one shared connection per path, a panic while holding the connection Mutex poisons it for the entire process lifetime — every subsequent op for that path then returns outcome ledger lock is poisoned. Previously each handler had an isolated connection. Transactions use RAII rollback so the risk is low, but the blast radius grew; consider clear_poison()/recovery or a doc note on the tradeoff.
Registry never evicts (acknowledged by design). Fine for production (bounded set of DB paths). In long-lived test processes entries accumulate, but the footprint is negligible.
busy_backoff shift bound. 1u64 << attempt.min(5) permits a shift of 5 while BUSY_RETRY_MAX_ATTEMPTS only drives attempt 1..=4; safe (.min(100) caps the value, no overflow) and the corrected doc comment now matches actual behavior (10/20/40/80ms).

Checklist

Code quality and standards — idiomatic, well-documented, structured logging
Test coverage adequate — targeted regression + full typed-OODA suite green
No TODOs, stubs, or swallowed exceptions — retries surface errors unchanged
No unimplemented functions
Logic correctness — verified registry routing, retry predicate, pragmas
Edge case handling — see non-blocking notes #1/#2 for defensive hardening

None of the observations block merge. Recommend merge once pre-commit/coverage CI complete.

rysweet · 2026-07-23T06:29:19Z

Step 17c — Security Review

Verdict: ✅ PASS (merge-ready) — 1 LOW-severity repo-hygiene finding, no exploitable vulnerabilities.

Reviewed commit cde7b9c9 against the security checklist. All automated security gates pass: GitGuardian, cargo-audit, cargo-deny, cargo-vet, npm-audit — all green.

Checklist results

Area	Result	Notes
Injection (SQL)	✅ PASS	Every `execute`/`query_row`/`query_map`/`prepare` in `ledger.rs` uses parameter binding (`params![…]`, `?1` placeholders, `[]`). No user input is string-interpolated into SQL. The two `format!` calls (L743/L750) build error-message strings only, never SQL.
Injection (shell/eval/deser)	✅ PASS	No `Command`/shell exec, no eval, no untrusted deserialization. WAL/foreign-key pragmas are static literals.
Sensitive-data handling	✅ PASS (code)	No secrets, tokens, keys, or credentials in code. Error paths use structured `tracing::error!` (replacing raw `eprintln!`) and carry only bounded, non-sensitive context.
AuthN / AuthZ	➖ N/A	Change is a local SQLite persistence layer; no auth surface introduced.
Path / traversal	✅ PASS	`canonical_key` canonicalizes the parent dir and rejoins the file name, falling back to the raw path on error. DB paths are internally derived (not attacker-controlled), so no traversal exploit. Registry lookup + `Connection::open` happen under the same mutex → no open-time TOCTOU.
Concurrency safety	✅ PASS	Shared `Arc<Mutex<Connection>>` per canonical path. Mutex poisoning is fail-visible — mapped to a persistence error, never `unwrap()`-panicked or silently ignored.
Error visibility	✅ PASS	Errors are bounded and surfaced; none are swallowed. Regression test asserts durable persistence (can't pass by dropping writes).

Finding — LOW: accidental commit of runtime session artifacts

Three files were committed that are not part of the feature:

.claude/.claude-4483-stash/runtime/launcher_context.json
.claude/.claude-4483-stash/runtime/metrics/post_tool_use_metrics.jsonl
.claude/.claude-4483-stash/runtime/sessions.jsonl

Root cause: .gitignore ignores /.claude/runtime/, but these live under /.claude/.claude-4483-stash/runtime/, which the anchored pattern does not match — so they slipped past the ignore rule.
Exposure: No secrets/tokens/credentials (verified by pattern scan + GitGuardian pass). They do contain local developer environment data: absolute paths (/home/azureuser/…), PIDs, session IDs, and internal workflow prompt logs. This is minor information disclosure / repo hygiene, not exploitable.
Severity: LOW — non-blocking. Recommend before/after merge:
1. git rm -r --cached .claude/.claude-4483-stash/ and drop from the PR, and
2. tighten .gitignore (e.g. /.claude/**/runtime/ or .claude/.claude-*-stash/) so stash runtime dirs can't be re-committed.

Non-security notes (already tracked)

Registry never evicts (bounded by distinct DB paths — trivial), and relative-vs-absolute path aliasing in canonical_key — both previously raised as non-blocking defensive-hardening items. No security impact.

Conclusion: No new vulnerabilities introduced. The path-keyed shared-connection fix is secure. The only actionable item is removing the accidentally-committed .claude runtime artifacts; it does not block merge.

rysweet · 2026-07-23T06:32:08Z

Step 17d — Philosophy Guardian Review

Verdict: ✅ PASS / merge-ready — the code change is strongly philosophy-compliant. One non-blocking repo-hygiene note (stray runtime artifacts).

Compliance checklist

Principle	Status	Evidence
Ruthless simplicity	✅	Fixes the root cause of #4483 directly: one shared `Arc<Mutex<Connection>>` per canonical DB path via a small process-wide registry, replacing N independent connections that contended on the file-level write lock. Minimal correct fix — no speculative machinery.
Bricks & studs	✅	Clean seam: `shared_connection` / `connection_registry` / `apply_connection_pragmas` are private helpers internal to `ledger.rs`. The public stud — `CapabilityHandler::open(path, policy)` — is unchanged, so callers are unaffected.
Zero-BS (no stubs / faked APIs / swallowed errors)	✅	No stubs or placeholders. Busy-retry surfaces the underlying `rusqlite::Error` unchanged as `PersistenceFailed` on exhaustion (`begin_immediate!` early-returns `persistence(err)`); nothing is swallowed. Mutex-poison maps to an explicit `PersistenceFailed` via `persistence_message`, not `.unwrap()`. Startup-recovery `eprintln!` upgraded to structured `tracing::error!` with `goal_id`/`error` fields.
No over-engineering	✅	`begin_immediate!` is a macro (not a fn) specifically because it must early-`return` the enclosing method's error type after retry exhaustion — a justified Rust idiom, not gratuitous magic. Backoff is a 6-line pure function. Registry has no eviction, TTL, or metrics layered on speculatively — appropriately scoped for a bounded set of long-lived DB paths.
Clean module boundaries	✅	Concurrency concern is fully encapsulated in `ledger.rs`; `typed_goal_session.rs` only swaps a print for structured logging. No new cross-module coupling.

Non-blocking observation (repo hygiene — not a code-quality issue)

Stray runtime artifacts committed. The PR includes .claude/.claude-4483-stash/runtime/ session state (launcher_context.json, metrics/post_tool_use_metrics.jsonl, sessions.jsonl — ~33 lines). These are ephemeral agent/session artifacts unrelated to the fix and mildly violate "keep the repo clean / ruthless simplicity." Recommend removing them from the branch (and confirming .claude/**/runtime/ is gitignored) before merge. Does not affect the shipped binary or tests.

Not changed (correct/intentional)

Retry logic, registry-no-eviction (by design for long-lived paths), public API signatures, and the WAL + 5s busy_timeout pragmas — all consistent with prior Pre-Commit / Security / Code reviews.

Conclusion: Philosophy-compliant. The one note is a trivial artifact cleanup, not a blocker.

Untrack .claude/.claude-4483-stash/runtime/ session artifacts that bypassed the root-only /.claude/runtime/ ignore rule, and add a recursive .claude/**/runtime/ rule to prevent recurrence. Addresses non-blocking repo-hygiene finding from PR #4495 reviews (Steps 17b/17c/17d). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

github-actions · 2026-07-23T06:51:22Z

📊 Coverage Summary

Generated by cargo llvm-cov --workspace --summary-only (nightly, excluding test files)

Module	Lines	Covered	Coverage
Total	194544	163498	84.0%

_{Coverage data from CI run. Test files matching tests?/ are excluded from line counts.}

…gistry key Quality-audit findings on the shared-connection registry (#4483): - Verify PRAGMA journal_mode=WAL actually engaged. SQLite reports the resulting mode without erroring when it cannot honor WAL (e.g. an unsupported filesystem); the prior code discarded that result and proceeded silently, defeating the concurrency fix's core guarantee. Now surfaces a PersistenceFailed error instead of a silent fallback. - Anchor a parent-less (bare relative) DB path to the current directory in canonical_key so it still canonicalizes to an absolute key, keeping the one-file/one-shared-connection invariant. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

rysweet and others added 4 commits July 23, 2026 03:58

wip: checkpoint after implementation (steps 7-8)

3f69bbd

Automatic checkpoint to preserve work in progress. Tests and implementation saved before refactoring phase.

rysweet commented Jul 23, 2026

View reviewed changes

rysweet mentioned this pull request Jul 24, 2026

Update documentation with 33 changed files (#4575) #4612

Draft

3 tasks

Navigation Menu

Update .claude with 11 changed files (#4483) - #4495

Update .claude with 11 changed files (#4483)#4495
rysweet wants to merge 6 commits into
mainfrom
feat/issue-4483-fix-systemic-typed-outcome-persistence-failed-data

rysweet commented Jul 23, 2026 •

edited

Loading

Uh oh!

rysweet left a comment

Uh oh!

rysweet commented Jul 23, 2026

Uh oh!

rysweet commented Jul 23, 2026

Uh oh!

github-actions Bot commented Jul 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

rysweet commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Issue

Changed files

Diff stat

Behavior

Validation

Risk

Checklist

Step 16b: Outside-In Testing Results

Uh oh!

rysweet left a comment

Choose a reason for hiding this comment

Step 17b — Comprehensive Code Review

Verification performed (fresh, on cde7b9c9)

Strengths

Non-blocking observations

Checklist

Uh oh!

rysweet commented Jul 23, 2026

Step 17c — Security Review

Checklist results

Finding — LOW: accidental commit of runtime session artifacts

Non-security notes (already tracked)

Uh oh!

rysweet commented Jul 23, 2026

Step 17d — Philosophy Guardian Review

Compliance checklist

Non-blocking observation (repo hygiene — not a code-quality issue)

Not changed (correct/intentional)

Uh oh!

github-actions Bot commented Jul 23, 2026

📊 Coverage Summary

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

rysweet commented Jul 23, 2026 •

edited

Loading

Verification performed (fresh, on `cde7b9c9`)